CodSpeed Performance Report

Merging #4859 will degrade performances by 88.05%

_{Comparing davidhewitt:no-err-normalization (71e0281) with main (da618c7)}

Summary

⚡ 14 improvements
❌ 2 regressions
✅ 82 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Looks like (so far at least) the tests pass ok.

The codspeed report suggests to me that:

• as above, we probably want to do this change alongside [RFC] split PyErr::new() into multiple methods #4413 so that we make it clear creating a PyErr needs the GIL
• the simplification to error state seems to have a noticeable impact across a lot of whole codebase, it seems desirable from a performance perspective to complete this

The performance regressions seem to come from two cases:

• Creating PyErr without the GIL now blocks on the GIL internally. I'm not worrried about this because I guess that will be a change to the PyErr API (maybe with deprecations).
• Cases where we create a PyErr (from a DowncastError, though could be any Rust error) and then immediately throw it away without checking the actual error content. Previously this would have just made a lazy error, but now it has to create a full Python exception for more wasted work.

I guess this also implies that we probably want to support a user-defined error type for FromPyObject, so that users have an option to avoid PyErr in such cases? (cc @Icxolu)

I guess this also implies that we probably want to support a user-defined error type for FromPyObject, so that users have an option to avoid PyErr in such cases?

That looks desirable to me, yes. I believe I have a branch somewhere that introduces FromPyObject::Error: Into<PyErr> that I can revive.

Awesome that this is a significant net decrease in LOC and hopefully it's rare to create and then discard a PyErr in a non-error code path.

Commenting to subscribe, but please feel free to ping me if you want my help with this.

Having rebased this, it's clear that this is still a pretty decent performance win across the codebase in general. With Error type for FromPyObject now implemented, with something for &str extract implementation (maybe #5391) we would not have the performance regression reported there and the only codepaths which might have regressions would be those which exploited PyErr's lazy behaviour.

Encouraging proper Rust errors over Python exceptions seems like good design for most codebases except where the error is always going out to Python anyway (in which case the laziness was purely an overhead).

The only question, IMO, is whether it's worth merging this to remove the behaviour from PyErr, or whether we should be introducing a new error handling mechanism and gracefully deprecate PyErr without disrupting users along the way. Given I don't think this is urgent, I'm satisfied to wait for now.